lib: require globals instead of using the global proxy #27112
Conversation
nodejs-github-bot
added
the
lib / src
|
@joyeecheung I am not sure what the benefit of using |
The latter is still subject to monkey-patching, so one could delete |
In addition, use process.stderr instead of console.error when there is no need to swallow the error.
joyeecheung
force-pushed
the
remove-globals
branch
from
3c95f90
to
24644ec
Compare
|
@BridgeAR Are your comments blocking? |
The builtins could still be directly manipulated (as in: the properties of the exports could be changed). For me the primordials are different since especially legacy code tends to add extra properties / functions on the prototype or changes behavior of some functions. Deleting properties from the global is something I never encountered so far (but it might still be done). But should we really guard against this case while not being able to guard against monkey patching the builtin in general?
I would like to at least discuss this a bit further. |
I think we should, or at least we should not start with letting things loose by default. If anyone actually has serious use case for this, we could explicitly use Specifically, what's the downside of guarding against monkey-patching like this? This is just a +42 −24 change, it's not really complex. |
|
@BridgeAR Does #27112 (comment) answer your question? |
|
Ping @BridgeAR |
|
Landed in a38e9c4 |
In addition, use process.stderr instead of console.error when there is no need to swallow the error. PR-URL: #27112 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
|
@BridgeAR, @joyeecheung, apologies for commenting on an old closed PR, but wanted to add a 👍for the change. Name clashes between globals can cause bugs in Electron's renderer process (like electron/electron#14915) when code in Node accidentally uses the global from Anyway, just throwing in my 2 cents on why this is a good PR. |
In addition, use process.stderr instead of console.error when there is no need to swallow the error. PR-URL: nodejs/node#27112 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
In addition, use process.stderr instead of console.error when
there is no need to swallow the error.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes